Merged
Conversation
art049
reviewed
Mar 25, 2025
Member
art049
left a comment
There was a problem hiding this comment.
A first quick review, I'll try to review more thouroughly
not-matthias
commented
Mar 26, 2025
GuillaumeLagrange
requested changes
Mar 26, 2025
There was a problem hiding this comment.
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/run/runner/wall_time/perf/fifo.rs:168
- Consider including the actual pid value in the error message to provide clearer context when the process lookup fails.
let bench_proc = procfs::process::Process::new(pid as _).expect("Failed to find benchmark process");
GuillaumeLagrange
requested changes
Apr 11, 2025
Contributor
GuillaumeLagrange
left a comment
There was a problem hiding this comment.
Great piece of work! Some overall small changes, next should be the last round of review!
95c91a1 to
6d4a041
Compare
art049
reviewed
Apr 25, 2025
art049
reviewed
Apr 25, 2025
art049
reviewed
Apr 25, 2025
6d4a041 to
8fca095
Compare
GuillaumeLagrange
approved these changes
Apr 25, 2025
Contributor
GuillaumeLagrange
left a comment
There was a problem hiding this comment.
Very minor comments appart from the multi-threading stuff we discussed IRL
8fca095 to
1b67cd8
Compare
bec9d0a to
26b46ad
Compare
94bebd2 to
e43a2ff
Compare
80d0929 to
73a079c
Compare
Member
Author
|
@GuillaumeLagrange Only minor changes in this PR: Added conditional dependency of |
GuillaumeLagrange
approved these changes
May 7, 2025
Contributor
GuillaumeLagrange
left a comment
There was a problem hiding this comment.
lgtm, good work!
73a079c to
4d6c062
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TODOs:
Don't use max freq (or parse it and send it with the traces)Actually doesn't matter. Higher freq, better callgraph.use correctcodspeedversion (depends on Cod 674 collect profiles while benchmarks are running codspeed-rust#92)Debug logs of successful run: